-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
task(department-name): Adiciona nome dos departamentos ao banco de dados #211
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #211 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 58 59 +1
Lines 2016 2027 +11
=========================================
+ Hits 2016 2027 +11 ☔ View full report in Codecov by Sentry. |
@@ -29,6 +29,7 @@ class Department(CustomModel): | |||
year:str -> Ano do departamento | |||
period:str -> Período do departamento | |||
""" | |||
name = models.CharField(max_length=128, default='') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encontrei um departamento que possui 123
caracteres: PROGRAMA DE PÓS-GRADUAÇÃO EM PROFNIT - PROPRIEDADE INTELECTUAL E TRANSFERÊNCIA DE TECNOLOGIA PARA A INOVAÇÃO (PROFISSIONAL)
.
Acredito que esse é um dos casos que podemos deixar o tamanho máximo um pouco maior para garantir (em longo prazo) a ausência de erros com esse sistema. O que acha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bom dia! Fiz algumas poucas considerações no PR. Quando puder, poderia dar uma olhada?
self.assertEqual(type(list()), type(departments)) | ||
self.assertEqual(type(tuple()), type(departments)) | ||
if len(departments): | ||
self.assertEqual(type(str()), type(departments[0])) | ||
self.assertEqual(type(list()), type(departments[0])) | ||
self.assertEqual(type(list()), type(departments[1])) | ||
self.assertEqual(type(str()), type(departments[0][0])) | ||
self.assertEqual(type(str()), type(departments[1][0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Para uma melhor legibilidade (e utilização de métodos que nos ajudam com isso), poderíamos utilizar o assertIsInstance.
return department_ids | ||
departments_names.append( | ||
option.get_text().replace("\n", "").replace("\r", "").replace("\t", "").replace("- BRASÍLIA", "")) | ||
return department_ids, departments_names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acredito que seria melhor o retorno (e uso) de uma única lista, contendo tuplas (id, name). Evitaria certas inconsistências e seria uma utilização mais fácil, sem depender do índice atual das listas.
|
||
return department_ids | ||
departments_names.append( | ||
option.get_text().replace("\n", "").replace("\r", "").replace("\t", "").replace("- BRASÍLIA", "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temos uma função para múltiplas substituições: multiple_replace.
Code Climate has analyzed commit fc76f36 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Descrição
**closes #109 **
Este PR adiciona o nome dos departamentos da UnB ao banco de dados. Foram feitas alterações nos códigos de WebScraping, Department Models e atualização/gerenciamento do banco de dados. Os testes foram atualizados para contemplar o nome dos departamentos.
Tipo de alteração
Como isso foi testado?
Os testes já existentes foram atualizados para contemplar o nome do departamento. Não houve necessidade de adicionar novos testes.
Checklist